Revert app harness subprocess#5610
Conversation
This reverts commit 9114d21.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR represents a significant architectural change to the testing infrastructure in Reflex, specifically reverting the app harness subprocess implementation. The changes move away from managing the backend server through subprocesses to a more direct, thread-based approach within the same process. Key modifications include:
- Removing subprocess-based backend management in AppHarness in favor of direct uvicorn server management
- Simplifying error logging by consolidating separate error logs into a single logging stream
- Streamlining command construction for Gunicorn and Granian servers in production mode
- Updating all integration tests to work with the new thread-based backend approach
- Improving test robustness by adding automatic console.error detection
The changes affect multiple test files and core utilities, making the testing infrastructure more maintainable and reliable while reducing complexity in process management and logging.
Confidence score: 4/5
- This PR is relatively safe to merge as it primarily affects testing infrastructure rather than production code.
- The high score reflects the comprehensive test updates and improved error handling, though the complexity of process management changes warrants careful review.
- Key files needing attention:
- reflex/testing.py: Core changes to AppHarness implementation
- reflex/utils/exec.py: Changes to server command construction
- reflex/utils/console.py: Error logging modifications
17 files reviewed, 3 comments
| if not external_scripts_path.exists(): | ||
| external_scripts_path.parent.mkdir(parents=True, exist_ok=True) | ||
| external_scripts_path.write_text(external_scripts) | ||
| Path("assets/external.js").write_text(external_scripts) |
There was a problem hiding this comment.
logic: If parent directory doesn't exist, write_text() will raise FileNotFoundError. Consider using parent.mkdir(exist_ok=True) or Path('assets').mkdir(exist_ok=True) before writing.
| Path("assets/external.js").write_text(external_scripts) | |
| Path("assets/external.js").parent.mkdir(parents=True, exist_ok=True) | |
| Path("assets/external.js").write_text(external_scripts) |
| ) | ||
| elif isinstance(client_side.state_manager, (StateManagerMemory, StateManagerDisk)): | ||
| client_side.state_manager.states.pop(token, None) | ||
| del client_side.state_manager.states[token] |
There was a problem hiding this comment.
style: direct dict deletion is more concise but could raise KeyError if token doesn't exist - consider dict.pop(token, None) for safety
| import pytest | ||
| from playwright.sync_api import Page, expect | ||
|
|
||
| import reflex as rx |
There was a problem hiding this comment.
style: Consider moving rx import to top of imports section with other third-party packages
CodSpeed Performance ReportMerging #5610 will not alter performanceComparing Summary
|
2 participants
No description provided.